Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

permission: add initial environment permission #48077

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

permission: add initial environment permission #48077

wants to merge 14 commits into from
+1,090 −94

Conversation

daeyeon
Copy link
Member

@daeyeon daeyeon commented May 19, 2023 •
edited
Loading

Add initial environment permission support. This restricts permission to access environment variables through process.env by using --allow-env flag.

usages

Proposed basic usages are as follows:

  • --allow-env : All environment variables are allowed.
  • --allow-env=HOST : HOST is allowed.
  • --allow-env=HOST,PORT : HOST and PORT are allowed.
  • --allow-env=DB_* : Env vars starting with DB_ are allowed.
  • --allow-env=DB_*,-DB_PASSWORD : All env vars starting with DB_ except DB_PASSWORD are allowed.
  • --allow-env=*,-DB_PASSWORD : All env vars except DB_PASSWORD are allowed.

process[env_private_symbol]

This is based on the idea of using a new privileged API for builtins to access the environment variables instead of process.env. It preserves current behaviors of process.env on the child processes and the worker threads by leveraging the existing native traps. This approach required manual changes to all internal uses of it.

WIP:

  • Removing code parsing patterns related to wildcard and -.
  • Adding flags to enable full access to envvars.
  • Updating TCs.
  • Updating documents.
  • Seeking possible ways to safely enumeration/cloning of the env object.

Refs: nodejs/security-wg#993

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

@daeyeon
permission: implement process.permission.has for env scope
f2bb9c4 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
process: add an env proxy used by builtins only
5daf0cb 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
permission: check env permission in proxy traps and clone
9538af6 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
test: allow other permission tests to access env vars
cb85eba 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
doc: add env permission
555bbf5 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/modules
  • @nodejs/net
  • @nodejs/security-wg
  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels May 19, 2023
tniessen
tniessen reviewed May 19, 2023
View reviewed changes
Copy link
Member

@tniessen tniessen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's great that enumerating properties of process.env throws. (Then again, it's already bad for DX that asynchronous file system APIs can now also throw synchronous permission errors, in addition to existing asynchronous permission errors.)

On the other hand, hiding environment variables can be problematic, too, of course.

Deno chose a somewhat elegant approach by not making Deno.env an object with enumerable environment variables. Deno.env.toObject() requires a special "all" permission.

RafaelGSS
RafaelGSS reviewed May 19, 2023
View reviewed changes
Copy link
Member

@RafaelGSS RafaelGSS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned by @tniessen, since we don't have a standard way to have access to env properties besides enumerated properties. I think this is the best we can do.

Regarding synchronous throws in asynchronous API. We can adjust it, but, permission is not the only throw-sync check that runs in async APIs (CHECK_NULL).

Amazing work! I'll review it with attention later. Why is it a draft? what's missing?

tniessen
tniessen reviewed May 19, 2023
View reviewed changes
test/parallel/test-permission-env-access-windows.js Outdated Show resolved Hide resolved
doc/api/permissions.md Outdated Show resolved Hide resolved
src/node_env_var.cc Outdated Show resolved Hide resolved
@daeyeon
fixup! doc: add env permission
cf09aaa 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon marked this pull request as ready for review May 19, 2023 14:57
@daeyeon
fixup! fixup! doc: add env permission
9ea7a9d 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
ljharb
ljharb reviewed May 19, 2023
View reviewed changes
lib/cluster.js
},
} = internalBinding('util');

const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you may want to use ObjectHasOwn for this, since potentially anyone could add NODE_UNIQUE_ID to the prototype chain.

Suggested change
const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary';
const childOrPrimary = ObjectHasOwn(process[env_private_symbol], 'NODE_UNIQUE_ID') ? 'child' : 'primary';

lib/internal/process/permission.js Outdated Show resolved Hide resolved
@Qard
Copy link
Member

Qard commented May 20, 2023

I feel like this might need a bit more thinking of how to handle enumeration/cloning of the env object safely. There's lots of cases of the process.env object getting cloned without any particular awareness of the contents to expect that it could suddenly fail. Perhaps it's better in enumeration to return undefined rather than throwing on blocked keys? I wonder if we could have a way to get a list of all present keys, even blocked ones, but not have access to map the key to the value?

@daeyeon daeyeon added the wip Issues and PRs that are still a work in progress. label May 20, 2023
@daeyeon
fixup! fixup! fixup! doc: add env permission
4ad57b0 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon mentioned this pull request May 22, 2023
Closed
@daeyeon
dev: update node_option --allow-env
373789a 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
dev: clean up env permission
2be253f 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
dev: update other permission tests to access env vars
64ef23e 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
dev: update tcs
9b5a4f3 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
ljharb
ljharb reviewed May 23, 2023
View reviewed changes
doc/api/cli.md Outdated Show resolved Hide resolved
@daeyeon
dev: add --allow-env-name
4f5e3f2 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon
dev: update docs
5f72c0f 
Signed-off-by: Daeyeon Jeong <daeyeon.dev@gmail.com>
@daeyeon daeyeon mentioned this pull request May 23, 2023
Merged
@RafaelGSS
Copy link
Member

RafaelGSS commented May 23, 2023 •
edited
Loading

Sorry delay, I didn't have time to jump into this discussion.

Deno was designed to facilitate a somewhat useful permission system from day one. Node.js wasn't. I don't think adding questionable features in an attempt to catch up with Deno should be a goal of the Node.js project.

I 100% agree. Honestly, after thinking for some time, I don't see a security reason to block the process.env.* access. It doesn't seem like a feature someone would use either in production or in development and Node.js doesn't have a history of exploits using environment variables. If we proceed with this restriction (--allow-env), it will mean there are hundreds of uncovered cases we'll need to deal with, and since this is a security feature, all those cases will be considered a vulnerability.

@Qard
Copy link
Member

Qard commented May 23, 2023 •
edited
Loading

Well, the env could contain sensitive data such as AWS credentials so I do think restricting access could make sense. Though as has already been expressed, there are tools to do that externally.

I think this may be a case of a solution looking for a problem though. I would suggest we hold off for now and reconsider later if users ask for it or a clear need comes up. New features can be added any time, but removing them is incredibly difficult.

@daeyeon
Copy link
Member Author

daeyeon commented May 24, 2023

I'd like to respectfully step back from my original thought and express my agreement with everyone's opinion. I think it's appropriate to hold this request. Thank you for taking the time to think about this together. 🙏

@tniessen tniessen added the permission Issues and PRs related to the Permission Model label Aug 10, 2023
@Kikobeats Kikobeats mentioned this pull request Sep 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb left review comments

@targos targos targos left review comments

@tniessen tniessen tniessen left review comments

@richardlau richardlau richardlau left review comments

@RafaelGSS RafaelGSS RafaelGSS left review comments

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. permission Issues and PRs related to the Permission Model wip Issues and PRs that are still a work in progress.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@daeyeon @nodejs-github-bot @Qard @RafaelGSS @ljharb @targos @tniessen @richardlau